Skip to content

Ci: assert declared production dependencies survive npm ci --omit=dev - #651

Open
AmaadMartin wants to merge 4 commits into
mainfrom
feat/ci-production-install-check
Open

Ci: assert declared production dependencies survive npm ci --omit=dev#651
AmaadMartin wants to merge 4 commits into
mainfrom
feat/ci-production-install-check

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A

  2. Or, if no issue exists, describe the change:

Problem: Nothing in CI ever performs a production install. run-tests runs
npm install, which installs devDependencies too, so a package the published
code imports at runtime still resolves there even when a production install
would prune it. A lockfile can be perfectly self-consistent — and pass a
lockfile drift guard — while resolving a published package's runtime dependency
to a node that npm ci --omit=dev removes. The end-user symptom is that the
package installs fine for contributors and throws on first import for
consumers.

Solution: Add one production-install job to validation.yaml:

- name: Install production dependencies only
  run: npm ci --omit=dev --ignore-scripts

- name: Check declared production dependencies are installed
  run: npm ls --omit=dev --workspaces --include-workspace-root

npm ls is the assertion, not a listing: it walks each workspace's declared
dependencies against the installed tree and exits non-zero with ELSPROBLEMS
when one is absent. --workspaces --include-workspace-root covers all three
published manifests plus the root. The check is manifest-driven, so it grows
automatically as dependencies are added, and there is no allowlist and no
package name anywhere in the diff.

This is a revision. The first version of this PR hand-rolled the same check as a
144-line scripts/check_production_install.mjs with a 248-line test suite; a
reviewer pointed out that npm already does it. Both files are deleted here — the
diff is now 24 added lines in one file, and those tests go away only because
their subject does, not because coverage was thinned. npm ls is also strictly
better on three axes the script gave up on: it names the version range and the
requiring package rather than two probed paths, it expands globbed workspaces
entries instead of refusing to run, and --include-workspace-root covers the
root manifest's own dependencies (empty today, so latent).

Two details that are load-bearing and easy to get wrong:

  • --ignore-scripts is required, not defensive. Without it,
    npm ci --omit=dev fails with code 127 / command sh -c husky: the root
    prepare script runs husky, which is a devDependency and is not installed
    here. It also keeps arbitrary postinstall code from running and keeps the job
    fast, and it does not weaken the assertion, since every package is still
    extracted.
  • ubuntu-latest only, deliberately. The declared dependency set is
    platform-independent, so a three-OS matrix would triple the cost for no extra
    signal. This is a choice, not an oversight.

Honest limits.

  • This validates the manifest -> installed tree direction only. A package that
    src imports but that appears in no manifest, or only in
    devDependencies, is not part of the tree npm walks, so this job stays green.
    Catching that needs an import-graph scan, which is separate work and out of
    scope. Calling this job "catches undeclared runtime imports" would be false.
  • npm ls is stricter than "is every declared dependency installed": on a
    --omit=dev tree it also reports invalid, extraneous and unmet peers as
    ELSPROBLEMS. That is stricter in the right direction for a production-install
    gate, and it is quiet on this tree today — exit 0 both locally and on the CI
    runner. If it ever turns noisy, that is the fact that would justify bespoke
    code; it is not true now.
  • Two small things the bespoke script had are gone: the ::error:: annotation
    in the GitHub UI (the failed step and the npm ERR! lines remain), and a
    "verified N dependencies" summary line. npm ls prints the resolved tree
    instead, which is more informative about what was actually checked.

Deliberate scope limits (nothing silently dropped): no dependency
declaration or lockfile change; no lockfile-drift step; no Node version pin, npm
cache, or concurrency group on the workflow; no import-graph scan; no npm script
alias.

Collision check. gh pr list --repo AmaadMartin/adk-js --state open --limit 1000
returned 550 open PRs; I also fetched every fork branch and grepped all of them
for omit=dev, --production, check_production_install and
production-install under .github/workflows/ and scripts/. No branch
performs a production install. The nearest neighbours, none of which this
duplicates or conflicts with:

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[ ] I have added or updated unit tests for my change.
[ ] All unit tests pass locally.

There are no unit tests, and that is the point of the revision: the change is a
workflow job invoking npm, with no repo code left to test. The job runs on every
PR, so CI exercises the real thing; a test asserting that npm ls behaves like
npm ls would only pin a mock. The change touches no source, so the existing
suites are unaffected. What I did instead is verify the command empirically
against a real production install of this repo, in a scratch clone (npm ci
deletes node_modules, so do not do this in your working checkout). The
absolute clone path is abbreviated to <clone> below.

1. Healthy production install — exit 0.

$ git clone <your-fork> <clone> && cd <clone>
$ git checkout feat/ci-production-install-check
$ npm ci --omit=dev --ignore-scripts
added 605 packages in 11s
$ npm ls --omit=dev --workspaces --include-workspace-root
adk@1.5.0 <clone>
├─┬ @google/adk-devtools@1.5.0 -> ./dev
│ ├── @clack/prompts@0.11.0
...
├─┬ @google/adk-integrations@1.5.0 -> ./integrations
│ └── @google/adk@1.5.0 deduped -> ./core
└─┬ @google/adk@1.5.0 -> ./core
  ├── adm-zip@0.5.17
...
$ echo $?
0

All 49 declared production dependencies are listed, including the two workspace
links that resolve through a symlink (@google/adk -> ./core, declared by both
dev and integrations), and no devDependency is needed for the command to
succeed. No invalid, extraneous or UNMET lines.

2. A declared dependency deleted — exit 1, named.

$ rm -rf node_modules/jsonpath-plus node_modules/camelcase-keys
$ npm ls --omit=dev --workspaces --include-workspace-root
npm ERR! code ELSPROBLEMS
npm ERR! extraneous: camelcase@5.3.1 <clone>/node_modules/camelcase
npm ERR! missing: camelcase-keys@^6.2.2, required by dev@npm:@google/adk-devtools@1.5.0
npm ERR! missing: jsonpath-plus@^10.4.0, required by core@npm:@google/adk@1.5.0
$ echo $?
1

Both failures are reported in one run, each naming the version range and the
requiring workspace.

3. The actual incident class — exit 1. This is the shape of the adm-zip
bug that motivates the job, and the case a lockfile drift guard does not catch:
a manifest declares the package under dependencies while its lockfile node
carries "dev": true, so the lockfile stays self-consistent and npm ci
succeeds but prunes the package. Reproduced by injecting "dev": true onto the
node_modules/jsonpath-plus lockfile node in the scratch clone:

$ npm ci --omit=dev --ignore-scripts     # succeeds
$ ls node_modules/jsonpath-plus
ls: cannot access 'node_modules/jsonpath-plus': No such file or directory
$ npm ls --omit=dev --workspaces --include-workspace-root
npm ERR! code ELSPROBLEMS
npm ERR! missing: jsonpath-plus@^10.4.0, required by core@npm:@google/adk@1.5.0
$ echo $?
1

4. --ignore-scripts is necessary — dropping it fails the install before
the check can run:

$ npm ci --omit=dev
npm ERR! code 127
npm ERR! command failed
npm ERR! command sh -c husky

5. On the CI runner (Node v22.23.1, ubuntu-latest). The new job on this PR's
own run: npm ci --omit=dev --ignore-scripts reported "added 598 packages, and
audited 602 packages in 12s", then
npm ls --omit=dev --workspaces --include-workspace-root printed the resolved
tree from adk@1.5.0 /home/runner/work/adk-js/adk-js with no ELSPROBLEMS,
invalid, extraneous or UNMET lines, and the job passed in 19s. That is the
evidence for the "stricter, but quiet on this tree" claim above, on the npm
version CI actually uses rather than only the one on my machine.

One unrelated red on this branch: run-tests (windows-latest) failed once on
app_loader_test.ts > should discover apps vs agents across directories and standalone files with Test timed out in 40000ms. That suite is untouched by
this diff (which is one workflow file), it is a known intermittent timeout on
the Windows and macOS runners with open fixes of its own, and it passed on
re-run. All checks are green now.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

The four blocks above are the manual test, each reproducible with the commands
shown. The job on this PR's own CI run is the same sequence on a clean runner;
its result is visible in the checks on this PR.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[ ] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 4 commits August 4, 2026 11:53
A lockfile can be self-consistent and still resolve a published package's
runtime dependency to a node that a production install prunes, so the
package installs fine for contributors and fails at first import for
consumers. Add a dependency-free check that reads every published
workspace manifest and probes the installed tree for each declared
dependency, plus its black-box test suite.

The check is driven off the manifests rather than an allowlist, so it
covers new dependencies automatically.
`run-tests` installs devDependencies, so a package reachable only through
the dev tree still resolves there. Add a separate ubuntu-only job that
performs a real production install and runs the check against it, which
is the only place the published dependency set is exercised.
A published workspace may legitimately carry no runtime dependencies, and
the fallback for a missing dependencies block was the one expression the
suite never executed.
…ript

npm already performs this check: `npm ls --omit=dev --workspaces
--include-workspace-root` walks every workspace's declared dependencies
against the installed tree and exits ELSPROBLEMS when one is absent. It
also names the version range and the requiring package, and expands
globbed workspace entries, which the hand-rolled script could not.

Verified against a production install of this repo: exit 0 on a healthy
tree; exit 1 naming the package when a declared dependency is deleted;
exit 1 when a lockfile node for a declared production dependency carries
"dev": true and npm ci --omit=dev prunes it, which is the incident this
job exists to catch.

Also revert an unrelated re-quoting of NODE_OPTIONS that a formatter
applied to the env block.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant